Skip to content

Conversation

@MaximilianLloyd
Copy link
Contributor

@MaximilianLloyd MaximilianLloyd commented May 12, 2023

In this code block, settings is fetched and then fetched again inside isExcluded inn a seperate call.

server.ts

    async onCompletion(params: CompletionParams): Promise<CompletionList> {
      return withFallback(async () => {
        if (!state.enabled) return null
        let document = documentService.getDocument(params.textDocument.uri)
        if (!document) return null
        let settings = await state.editor.getConfiguration(document.uri)
        if (!settings.tailwindCSS.suggestions) return null
        if (await isExcluded(state, document)) return null
        return doComplete(state, document, params.position, params.context)
      }, null)
    },

isExcluded.ts

export default async function isExcluded(
  state: State,
  document: TextDocument,
  file: string = getFileFsPath(document.uri)
): Promise<boolean> {
 // Here the exact same settings are fetched again
  let settings = await state.editor.getConfiguration(document.uri)

  for (let pattern of settings.tailwindCSS.files.exclude) {
    if (minimatch(file, path.join(state.editor.folder, pattern))) {
      return true
    }
  }

  return false
}

Here it is with my changes

server.ts

async onCompletion(params: CompletionParams): Promise<CompletionList> {
      return withFallback(async () => {
        if (!state.enabled) return null
        let document = documentService.getDocument(params.textDocument.uri)
        if (!document) return null
        let settings = await state.editor.getConfiguration(document.uri)
        if (!settings.tailwindCSS.suggestions) return null
        if (await isExcludedOnComplete(state, document, settings)) return null
        return doComplete(state, document, params.position, params.context)
      }, null)
    },

isExluded.ts

export async function isExcludedOnComplete(
  state: State,
  document: TextDocument,
  settings: Settings,
  file: string = getFileFsPath(document.uri)
): Promise<boolean> {
  for (let pattern of settings.tailwindCSS.files.exclude) {
    if (minimatch(file, path.join(state.editor.folder, pattern))) {
      return true
    }
  }

  return false
}

Doing this halves the hits to the cache 2 -> 1, which has quite a big impact since the document settings cache is quite large.

From some preliminary benchmarking it takes about 100ms to get a settings object after it's cached. So eliminating a 100ms call is quite valuable.

@bradlc
Copy link
Contributor

bradlc commented May 15, 2023

Hey @MaximilianLloyd. Appreciate the PR but I think I'm going to leave this as it is for now. Having said that it should definitely not be taking anywhere near 100ms to read the settings from the cache 😳 How many entries are in the cache when you're seeing times like that?

@bradlc bradlc closed this May 15, 2023
@MaximilianLloyd
Copy link
Contributor Author

I've probably misread the values a bit. But the document settings cache can become quite large, as it seems to store it per file. Especially when working on a project for a sustained period.

Nonetheless avoiding hitting that twice would be beneficial?


Anyways i'd be happy to happy to contribute on something else if you have any suggestions, so that is relevant for you 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants